Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change tda to non tda and vice versa #4367

Merged
merged 4 commits into from
Jul 12, 2024

Conversation

tomas-stefano
Copy link
Contributor

@tomas-stefano tomas-stefano commented Jul 11, 2024

Context

There were 4 bug cards from the bug party:

https://trello.com/c/9VaxXZ1f/2004-bug-investigate-recreating-403-access-issue-you-are-not-permitted-to-see-this-page-when-switching-a-tda-course-to-non-tda?filter=label:Squad%20B
https://trello.com/c/5HipohNv/2007-bug-investigate-broken-tda-course-that-wont-publish
https://trello.com/c/YuJcPxuE/1968-tda-publish-edge-case-reset-the-a-levels-when-user-changes-from-a-tda-course-to-a-non-tda-course
https://trello.com/c/n791FkBt/2013-bug-changing-back-to-tda-having-changed-to-non-tda-ends-up-being-unpublishable

These are always part of the same problem:

  1. Changing TDA to non TDA course (in draft/rollover state).
  2. Changing non TDA to TDA course (in draft/rollover state).
  3. Changing TDA to non TDA course (in published state).
  4. Changing non TDA to TDA course (in published state).

Changes implemented

We should allow to change from TDA <-> non TDA when draft or rollover. But not when the course is published.

Guidance to review

  1. Changing TDA to non TDA in draft/rollover state - can this be published?
  2. Is the link hidden when is TDA published course?
  3. Is the TDA option hidden when changing qualification on a published NON TDA course?

When changing from non TDA to TDA course:
  Make sure full time is set on site statuses

When changing from TDA to non TDA course:
  Make sure A level is clear
Also hide the change outcome links when the course is TDA and published
@tomas-stefano tomas-stefano marked this pull request as ready for review July 12, 2024 09:13
else
raise "Unexpected study mode #{study_mode}"
end
end
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was in a controller concern and was included in the Course model (only on course) so I remove the module and move the code here.


qts_list - %w[undergraduate_degree_with_qts]
qts_list
Copy link
Contributor Author

@tomas-stefano tomas-stefano Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really wish one day to refactor the whole Edit options code because at least to me sounds wrong changing this module to remove an option in the view. And changing this module will affect different screens that uses the edit options.

I think the EditOption abstraction is bringing more problems than solutions. But maybe is just me.

To solve this bug I have to add the condition above.

Copy link
Contributor

@CatalinVoineag CatalinVoineag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good to me. And most of the bugs have been fixed

I still managed to change the course from TDA to something else when it's published but not as easy. Feels like something is changing when I get pundit policy tries to stop me.

Peek.2024-07-12.11-15.mp4

@tomas-stefano
Copy link
Contributor Author

The code looks good to me. And most of the bugs have been fixed

I still managed to change the course from TDA to something else when it's published but not as easy. Feels like something is changing when I get pundit policy tries to stop me.

Peek.2024-07-12.11-15.mp4

Interesting. I did test for the published but not when is scheduled state. 🤔
Thank you for spotting. I will add one feature test for this.

@tomas-stefano
Copy link
Contributor Author

@CatalinVoineag I pushed the fix. I optin to move everything to Pundit but to redirect away if you enter manually on the page.

Copy link
Contributor

@CatalinVoineag CatalinVoineag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍🏻

@tomas-stefano tomas-stefano merged commit ca30548 into main Jul 12, 2024
19 checks passed
@tomas-stefano tomas-stefano deleted the change-tda-to-non-tda-and-vice-versa branch July 12, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants